-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce entity animations #806
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few things mostly with docs. Is State an appropriate name for the field & methods considering it's actually the next state?
@@ -915,9 +917,12 @@ func (s *Session) ViewEntityState(e world.Entity) { | |||
} | |||
|
|||
// ViewEntityAnimation ... | |||
func (s *Session) ViewEntityAnimation(e world.Entity, animationName string) { | |||
func (s *Session) ViewEntityAnimation(e world.Entity, animation animation.Animation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just rename this variable to a to avoid conflicting with the package, same thing in the interface yupe in viewer.go
return Animation{ | ||
name: animationName, | ||
state: "", | ||
controller: "", | ||
stopCondition: "", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be Animation{name: name}
// Animation represents an animation & controller that may be attached to an entity. | ||
// Animations and controllers must be defined in a resource pack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this would sound better
// Animation represents an animation that may be played on an entity from an active resource pack on
// the client.
// WithController sets the controller with the specified state. | ||
// The controller must be added in a resource pack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// WithController returns a copy of the Animation with the provided animation controller. The
// controller must be defined in a resource pack for it to work.
// Controller returns the name of the controller being used. Controller returns an empty string if | ||
// no controller was previously set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think it's necessary to mention the empty string here, also purely just for OCD reasons could you move Controller(), State() and StopCondition() methods to be before their respective With...() methods?
return a.controller | ||
} | ||
|
||
// WithState sets the state to transition to as defined in the controller. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// WithState returns a copy of the Animation with the provided next state.
// State returns the current state being played. State returns an empty string if | ||
// no controller was previously set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing, no point mentioning empty string. This is also incorrect since it's not the state that is currently being played. Something like this may be better:
// State returns the name of the next state in the animation controller.
return a.state | ||
} | ||
|
||
// WithStopCondition takes the molang expression and stops the animation if the query passes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// WithStopCondition returns a copy of the Animation with the provided stop condition. This condition is
// usually a Molang query which is constantly compared against until the animation has stopped.
// StopCondition returns the stop condition. StopCondition returns an empty string if | ||
// no molang expression was set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing, no point mentioning empty string
// AnimateEntity will start an animation for the specified entity. Viewers that are viewing the entity will be | ||
// played the animation. | ||
func (w *World) AnimateEntity(e Entity, animation animation.Animation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlayEntityAnimation
might be a better name to fit with PlaySound
etc.
Closing in favour of #940 |
The documentation probably needs improvement. Also tie-in pr for the wiki
Tests
Test resource pack: EntityAnimationTest.zip